Skip to content

refactor: eliminate uncors wrapper, wire server directly through cli - #97

Open
evg4b wants to merge 30 commits into
mainfrom
commands
Open

refactor: eliminate uncors wrapper, wire server directly through cli#97
evg4b wants to merge 30 commits into
mainfrom
commands

Conversation

@evg4b

@evg4b evg4b commented Jun 23, 2026

Copy link
Copy Markdown
Owner

Summary

  • Remove internal/uncors package — the Uncors wrapper around server.Server was a thin indirection layer with no independent value; deleted along with its ~1 300-line test suite
  • UncorsApp uses server.Server directly — proxy start/restart/shutdown logic inlined; mappingsToTarget extracted to di.Container.Targets() to remove duplication across callers
  • RunUncors is context-awarecli.RunUncors(ctx, fs, args) now accepts a context.Context; the shutdown goroutine triggers on both OS signals and ctx.Done(), so cancellation always stops the server cleanly
  • Integration tests boot via cli.RunUncorsbootProxy serialises the programmatic config to YAML, writes it to the test memfs, and starts the proxy through the real cli.RunUncors path (same as production); readiness is confirmed by polling mapped TCP ports
  • UNCORS_LOGGING env var — set to a file path to redirect log.* output there; empty (default) discards it as before
  • Linter fixes — resolved errcheck, forbidigo, gosec, noinlineerr, and varnamelen findings

Test plan

  • make check passes (format + unit tests + build)
  • make test-integration passes with the new bootProxy implementation
  • UNCORS_LOGGING=uncors.log ./uncors ... writes debug lines to the file
  • ./uncors --from http://localhost:3000 --to https://example.com starts normally (no regression)

🤖 Generated with Claude Code

evg4b and others added 11 commits June 22, 2026 23:33
…ncors wrapper

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…Targets

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- RunUncors and runNonIneractive now accept a context.Context so callers
  (including tests) can control server lifetime via cancellation
- Shutdown goroutine handles both OS signals and ctx.Done, ensuring the
  server stops cleanly in either case
- Removed duplicate mappingsToTarget from run_non_ineractive.go; now
  uses container.Targets(cfg) consistently
- bootProxy rewrites to serialize the config to YAML, call cli.RunUncors
  in a goroutine with t.Context(), and poll mapped ports for readiness

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- RunUncors defaults to interactive mode (defaultConfig sets Interactive:true);
  bootProxy must override with --interactive=false so the proxy starts its
  TCP listeners instead of launching the TUI
- Fix gofmt alignment in const block
- Replace 0o644 magic number with named constant configFilePerm
- Replace net.DialTimeout with (*net.Dialer).DialContext (noctx linter)
- Rename loop variable m -> mapping (varnamelen linter)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
yaml.Marshal serializes zero-value CacheConfig fields
(expiration-time: 0s, max-size: 0, methods: []), which override
defaultConfig() when the YAML is reloaded, causing integration
test boot to fail with CacheConfig validation errors.

omitempty on individual fields ensures zero values are omitted from
the serialized YAML, preserving the defaults on reload.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds tests for RunUncors (load error, startup/shutdown, port-in-use, config
reload success and error paths) and GenerateCerts, covering the new internal/cli
package. Extends di public_api_test with TestContainerTargets (HTTP, HTTPS,
port grouping). Adds main_test.go covering setupLogging and main() code paths.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@evg4b

evg4b commented Jun 23, 2026

Copy link
Copy Markdown
Owner Author

Added tests for internal/cli (RunUncors + GenerateCerts), internal/di Targets, and main/setupLogging — new-code coverage is now substantially higher across these packages.

🤖 Addressed by Claude Code

…fields

RewritingOption.Host (urlt.Host) serialized as 'host: ""' without omitempty,
causing ParseHost("") -> "empty host" on reload. Adding omitempty skips the
field when zero.

Script.MarshalYAML trims leading/trailing whitespace from the inline Script
string and flattens the inline Matcher fields into a plain struct. This avoids
a yaml.v3 bug where strings starting with \n produce |4 block scalars with
incorrect content indentation, breaking the Marshal/Unmarshal round-trip used
by the integration test harness.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@evg4b

evg4b commented Jun 23, 2026

Copy link
Copy Markdown
Owner Author

Fixed two integration test root causes (both were yaml.v3 round-trip bugs exposed by the config serialization in the test harness):

  1. RewritingOption.Host missing omitempty → serialized as host: ""ParseHost("") = "empty host" on reload
  2. Script.MarshalYAML now trims leading/trailing whitespace from inline script strings, avoiding a yaml.v3 bug where strings starting with \n produce |4 block scalars with wrong content indentation

🤖 Addressed by Claude Code

…d diagnostic

- TestScript_MarshalYAML: round-trip tests for inline script (with leading
  newline) and matcher field preservation, covering the MarshalYAML fix
- TestNewConfigWatcher: three new subtests for errAlreadyWatching, empty path,
  and Close-without-Watch code paths (watcher.go branch coverage)
- commands_test.go: add t.Logf diagnostic before require.NoError to capture
  the error value on failure, stabilising the reload subtest timing

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@evg4b

evg4b commented Jun 23, 2026

Copy link
Copy Markdown
Owner Author

Added tests targeting the specific uncovered new-code lines:

  • TestScript_MarshalYAML — covers the new MarshalYAML method added to fix the yaml.v3 round-trip bug (was 0% covered)
  • TestNewConfigWatcher/returns_errAlreadyWatching — covers the double-Watch error path
  • TestNewConfigWatcher/Watch_with_empty_path — covers the early-return for empty config path
  • TestNewConfigWatcher/Close_without_Watch — covers Close before Watch is ever called

🤖 Addressed by Claude Code

evg4b and others added 2 commits June 23, 2026 18:57
Remove the --debug CLI flag and debug config field. Logging is now controlled
exclusively via the UNCORS_LOGGING environment variable, simplifying the API.

Updates tests and documentation to reflect this change.

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Server.Shutdown could race with Server.Start (called via Restart) because
both access s.listeners without synchronisation. Introduce a sync.RWMutex:
Start holds the write lock while replacing the slice, Shutdown and Close
snapshot the slice under the read lock before iterating.

Also add internal/infra/loggings_test.go so that the SetupLogging function
(moved here from main.go in the previous refactor) is covered at the package
level rather than only through main_test.go cross-package calls.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@evg4b

evg4b commented Jun 23, 2026

Copy link
Copy Markdown
Owner Author

Added internal/infra/loggings_test.go with three tests covering all branches of SetupLogging (empty env, valid path, unwritable path). The function was moved from main.go to internal/infra/loggings.go in the previous refactor commit but had no package-level tests, leaving it at 0% coverage in the infra package.

Also fixed a data race in Server.Shutdown / Server.Start: both access s.listeners concurrently when a config reload fires while a shutdown is in progress. Added sync.RWMutex to protect the slice.

🤖 Addressed by Claude Code

evg4b and others added 11 commits June 23, 2026 19:16
Move TestGenerateCerts to generate_certs_test.go and TestRunUncors
to run_uncors_test.go. Tests are now co-located with the functions
they test, matching the split of commands.go into separate modules.

Also move helper functions (httpMapping, waitForPort, writeConfig,
startProxy) to run_uncors_test.go where they are used.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- generate_certs.go: GenerateCerts function for CLI cert generation
- run_uncors.go: RunUncors entry point that routes to interactive/non-interactive
- run_ineractive.go: runIneractive for TUI mode (BubbleTea-based)
- run_non_ineractive.go: runNonIneractive for headless mode with config watching

Also:
- Remove --debug CLI flag; logging now controlled solely via UNCORS_LOGGING env var
- Rename parameters for consistency: uncorsConfig→cfg, configPath→cfgPath
- Add context.Context to runIneractive for proper lifecycle management

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…orbidigo/nlreturn/wsl_v5)

- run_uncors.go: replace forbidden println with fmt.Fprintln(os.Stdout, ...)
  and add blank line before return nil after version print
- config.go: add blank line before if printVersion block (wsl_v5)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- RunUncors: add tests for --version (prints version, returns nil) and
  --help (returns nil via pflag.ErrHelp)
- GenerateCerts: add test for --help flag returning nil
- LoadConfiguration: add test confirming --version returns ErrVersionRequested

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
evg4b and others added 4 commits June 28, 2026 14:17
TestMain_RunUncorsErrorPath called main() with invalid args causing
handleError to call os.Exit(1), which kills the test process. Similarly
TestMain_GenerateCertsErrorPath triggered os.Exit via --unknown-flag.

Replace both with --version and --help paths that return nil and exit
main() normally, testing the same main() control flow without crashing
the test runner.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…process

Extract os.Exit into a package-level var so tests can replace it.
Add TestHandleError_ExitsOnError and TestHandleError_NoopOnNil to
cover the previously uncovered error branch, restoring coverage above 80%.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@sonarqubecloud

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant